Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Submodule for testing utils #10

Merged
merged 4 commits into from
Jul 7, 2020
Merged

Conversation

jlumpe
Copy link
Contributor

@jlumpe jlumpe commented Jul 6, 2020

Finally, the main reason I've been contributing to this package. Added Neighborhood.Testing submodule with functions that test the return value of search and related funcs.

I'm pretty sure all searches should return identical results regardless of the search structure used (right?), so I think these should be just about all the tests you need when adding a new SS type.

One thing I'm not sure about - are results expected to be sorted by distance? If not I need to change that before this is merged.

@Datseris
Copy link
Member

Datseris commented Jul 6, 2020

are results expected to be sorted by distance?

Nope, not generally. If you have a look at the NearestNeighbors implementation here: https://github.com/JuliaNeighbors/Neighborhood.jl/blob/master/src/kdtree.jl#L20 I added the possibility to sort as an argument. Because depending on the implementation, sorting by distance most of the time costs additional processing, so therefore it should always be optional to not do it.

@Datseris
Copy link
Member

Datseris commented Jul 6, 2020

If someone wants to contribute a new package to Neighborhood.jl, do they have to extend the testing?

@jlumpe
Copy link
Contributor Author

jlumpe commented Jul 6, 2020

If you've implemented the Neighborhood.jl API, the following should be pretty comprehensive for your tests:

using MyPackage, Test, Neighborhood
using Neighborhood.Testing

metric = ...
data = ...
ss = searchstructure(MyStructure, data, metric)

queries = ...

searches = [NeighborNumber(k), WithinRange(r)]


@testset "Single search"
    for query in queries
	for t in searches
	    for skip in [nothing, myskipfunc]
		# Calls search(), isearch(), knn() and checks results match
		idxs, ds = result = search_allfuncs(ss, query, t, skip)
		
		# Exact test for when brute force method isn't too slow
		@test result = bruteforcesearch(data, metric, query, t, skip)
		
		# Quicker alternative which just runs some sanity checks on results
		check_search_results(data, metric, results, query, t, skip)
	    end
	end
    end
end


@testset "Bulk search" begin
    for t in searches
	for skip in [nothing, mybulkskipfunc]
	    # Calls bulksearch(), bulkisearch(), and then search() on each query and compares
	    test_bulksearch(ss, queries, t, skip)
	end
    end
end

bruteforcesearch is in my next PR. I'll also need to add a function to this one that compares results up to a reordering and use it instead of straight equality testing.

@Datseris
Copy link
Member

Datseris commented Jul 7, 2020

Okay. Meanwhile I gave you admin rights in this repo, so you can branch directly from here instead of maintining a different fork.

@jlumpe
Copy link
Contributor Author

jlumpe commented Jul 7, 2020

Thanks. This should be good to go. You can see usage in the bruteforce branch.

@Datseris Datseris merged commit 1d54cd2 into JuliaNeighbors:master Jul 7, 2020
@Datseris
Copy link
Member

Datseris commented Jul 7, 2020

Awesome. We should put a note in the bruteforce PR saying something like "See the BruteForce tests for an example usage" at the dev docs.

@jlumpe jlumpe deleted the testing branch July 8, 2020 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants